Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update lookup model in console #15472

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

pranavbhole
Copy link
Contributor

@pranavbhole pranavbhole commented Dec 2, 2023

Description

Adding optional jitterSeconds, loadTimeoutSeconds,maxHeapPercentage to jdbc lookup add/edit forms. We had added first 2 params recently https://github.com/apache/druid/blob/master/docs/development/extensions-core/lookups-cached-global.md#jdbc-lookup specifically for jdbc.

Screenshot 2023-12-02 at 11 39 21 AM

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

Copy link
Contributor

@vogievetsky vogievetsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

Copy link
Contributor

@vogievetsky vogievetsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, what does 30 (optional), default is 0 mean?

@pranavbhole
Copy link
Contributor Author

Wait, what does 30 (optional), default is 0 mean?

30 is placeholder value which is recommended to set to starts with. May be placeholder string needs to be amended to depict it correctly. Let me know if you have suggestions.,

@@ -464,30 +464,30 @@ export const LOOKUP_FIELDS: Field<LookupSpec>[] = [
{
name: 'extractionNamespace.jitterSeconds',
type: 'number',
placeholder: '30 (optional), default is 0',
placeholder: '30 (optional) ',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of setting the placeholder this you could just add defaultValue: 30 this basically sets the placeholder to 30. The (optional) label is not needed because visually when you open the form and there is a default value filled in as the placeholder it visually conveys "optional". The placeholder is really there for when you need to override the default behavior that I don't think you need to do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed
Screenshot 2023-12-04 at 5 18 31 PM

@vogievetsky
Copy link
Contributor

thank you for address in the various feedbacks!

@vogievetsky vogievetsky merged commit 82e3c61 into apache:master Dec 5, 2023
11 checks passed
@pranavbhole pranavbhole deleted the updateLookupsModel branch December 6, 2023 05:08
@LakshSingla LakshSingla added this to the 29.0.0 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants